Fix UnsupportedOperationException when merging ActivityOptions with i…#2749
Fix UnsupportedOperationException when merging ActivityOptions with i…#2749maciejdudko merged 1 commit intotemporalio:masterfrom
Conversation
|
|
maciejdudko
left a comment
There was a problem hiding this comment.
Hi Nitish, thank you for your contribution. The implementation looks good, but the test needs an improvement, see the review comment.
temporal-sdk/src/test/java/io/temporal/activity/ActivityOptionsTest.java
Show resolved
Hide resolved
|
@maciejdudko Thanks for the feedback! Could you please re-review when you get a chance? |
temporal-sdk/src/test/java/io/temporal/activity/ActivityOptionsTest.java
Outdated
Show resolved
Hide resolved
|
@maciejdudko Updated based on feedback. Take a look when you get a chance. |
|
The PR looks good, but the Features Integration Tests CI job has failed. It looks like you haven't copied tags from our repository into your fork - without that, this job will not work. Please run the following commands inside your repository clone: Please leave a comment when you've run the commands so we can re-run CI. |
…mmutable context propagators The mergeActivityOptions method was calling addAll() directly on the existing contextPropagators list, which fails when the list is immutable (e.g., created with List.of() or Collections.emptyList()). This fix creates a new ArrayList that combines both lists instead of modifying the existing one. Fixes temporalio#2482
|
@maciejdudko Tags have been pushed to the fork and the branch has been rebased on the latest master. Please re-run the CI. |
maciejdudko
left a comment
There was a problem hiding this comment.
All looks good! Thank you for your contribution.
…mmutable context propagators (temporalio#2749)
What was changed
Modified
ActivityOptions.Builder.mergeActivityOptions()to create a newArrayListwhen combining context propagators instead of callingaddAll()directly on the existing list. Added a test to verify the fix.Files changed:
temporal-sdk/src/main/java/io/temporal/activity/ActivityOptions.javatemporal-sdk/src/test/java/io/temporal/activity/ActivityOptionsTest.javaWhy?
When users set context propagators using immutable lists (e.g.,
List.of()orCollections.emptyList()), mergingActivityOptionswould throwUnsupportedOperationExceptionbecause the code attempted to calladdAll()on an immutable list.This fix follows the approach suggested by @cretz in the issue: create a new list that combines both instead of modifying one of them.
Checklist
Closes Merging ActivityOptions fails with UnsupportedOperationException when using immutable lists for context propagators #2482
How was this tested:
testActivityOptionsMergeWithImmutableContextPropagatorsthat creates twoActivityOptionswith immutable context propagator lists (Collections.emptyList()) and verifies they can be merged without throwingUnsupportedOperationExceptionActivityOptionsTesttests pass./gradlew clean build)Any docs updates needed?